Dependency Injection Code And Documentation#7
Dependency Injection Code And Documentation#7anitconsultantllc wants to merge 2 commits intopostsharp:masterfrom
Conversation
…postSharp/PostSharp.Samples, "Missing Advanced Code Samples For Dependency Injection".
Added documentation for Missing Advanced Code Samples For Dependency Injection postsharp#6.
|
Thank you. Sorry for the delay due to the holiday season here. |
gfraiteur
left a comment
gfraiteur
left a comment
There was a problem hiding this comment.
Thank you. That looks interesting. It has been a long time since I wrote the documentation (it was actually before we published the samples) and in the meantime I realized that we need to cope with the problem of initialization ordering, i.e. your aspect (especially a logging aspect) may be requested to execute before the service locator was initialized. I think a sample project should be better equipped to cope with this situation. I guess it's my only substantial comment.
I understand you may not have the time to address this issue as it would rather be our responsibility to update the documentation. It's up to you,
If you choose to ignore this, then please just format the code according to the rest of the project and to .editorconfig. Thank you.
|
|
||
| namespace PostSharp.Samples.DependencyResolution.Contextual.Test | ||
| { | ||
| [TestClass] |
There was a problem hiding this comment.
Please split all files into single-type files because it's our convention.
| <packages> | ||
| <package id="MSTest.TestAdapter" version="1.3.2" targetFramework="net472" /> | ||
| <package id="MSTest.TestFramework" version="1.3.2" targetFramework="net472" /> | ||
| <package id="PostSharp" version="5.0.37" targetFramework="net472" developmentDependency="true" /> |
There was a problem hiding this comment.
Please use the same version of PostSharp as other projects.
| } | ||
|
|
||
| public static Lazy<T> GetService<T>(Type aspectType, MemberInfo targetElement) where T : class | ||
| { |
There was a problem hiding this comment.
I'm not sure Lazy is the right thing here. We need to cope with the problem of random initialization order, i.e. your aspect may be executed before the catalogue is initialized (especially with logging). One way would be to return an object of type say Service<T> where service.Value would return null before initialization, but after initialization, the value would be cached.
| public override void OnEntry(MethodExecutionArgs args) | ||
| { | ||
| logger.Value.Log("OnEntry"); | ||
| } |
There was a problem hiding this comment.
So you would do logger.Value?.Log, i.e. at runtime the Value could be null.
| { | ||
| if (isCacheable) | ||
| { | ||
| return () => new Lazy<T>(GetServiceImpl<T>).Value; |
There was a problem hiding this comment.
Nice :) It may be worth mentioning in a comment that you're using a compiler trick to make things cacheable. It didn't appear to me at first reading.
| public override void OnEntry(MethodExecutionArgs args) | ||
| { | ||
| logger().Log("OnEntry"); | ||
| } |
There was a problem hiding this comment.
Here also, use Elvis, logger()?.Log to cope with initialization order issue.
| private static T GetServiceImpl<T>() | ||
| { | ||
| if (container == null) | ||
| throw new InvalidOperationException(); |
There was a problem hiding this comment.
Cope with the initialization ordering issue. You don't want to fail with an exception if there is an attempt to log before initialization. Instead, you want logging to be silently skipped.
| public static void BuildObject(object o) | ||
| { | ||
| if (container == null) | ||
| throw new InvalidOperationException(); |
There was a problem hiding this comment.
Same problem of random initialization order here. One possible solution is that, if the container has not been initialized yet, you would store the object into a queue, which you would then initialize in the Initialize method. The aspect would need to cope with the situation that it has not been initialized yet.
|
|
||
| public static Lazy<T> GetService<T>() where T : class | ||
| { | ||
| return new Lazy<T>(GetServiceImpl<T>); |
There was a problem hiding this comment.
Same problem of initialization ordering.
2 participants
Please consider adding these changes to you PostSharp.Samples collection and update the documentation accordingly. The first of four examples here helped solve my issue and decreased development time, Global Service Container